Skip to content

fix(inquirerer): defer process.stdin/stdout access in defaultCLIOptions#78

Open
pyramation wants to merge 1 commit intomainfrom
feat/inquirerer-lazy-stdio
Open

fix(inquirerer): defer process.stdin/stdout access in defaultCLIOptions#78
pyramation wants to merge 1 commit intomainfrom
feat/inquirerer-lazy-stdio

Conversation

@pyramation
Copy link
Copy Markdown
Contributor

Summary

inquirerer/src/commander.ts defined defaultCLIOptions with two eager properties:

input: process.stdin,
output: process.stdout,

process.stdin / process.stdout are Node getters that lazy-construct the underlying Readable / Writable on first access. When fd 0 is a pipe (Jest workers, spawned children, any backgrounded process — i.e. the common case for anything running under jest), constructing stdin registers a libuv PIPEWRAP handle that the runtime must close before exit.

Because those properties were evaluated at module-load time, any library that transitively required inquirerer without ever constructing a CLI still opened stdin as a pure import side-effect. The chain that actually burned us:

pgsql-test/index.js:18     →  require("./admin")
pgsql-client/admin.js:4    →  require("@pgpmjs/core")
@pgpmjs/core/index.js:18   →  require("./core/class/pgpm")
template-scaffold.js:11    →  require("genomic")
genomic/index.js:19        →  require("inquirerer")
inquirerer/commander.js:14 →  input: process.stdin   ← leak

That surfaced in peptide-app-demo#40 as a PIPEWRAP open-handle warning from jest --detectOpenHandles even though no test was doing anything CLI-related.

Fix

Convert input and output into accessor properties (getters) so process.stdin / process.stdout are only touched when someone actually reads them (i.e. when new CLI(...) runs through commander.ts:43-46). Behaviour is unchanged for direct consumers — defaultCLIOptions.input still returns process.stdin on read.

export const defaultCLIOptions: CLIOptions = {
  version: `inquirerer@${getVersion()}`,
  noTty: false,
  get input()  { return process.stdin;  },
  get output() { return process.stdout; },
  minimistOpts: { alias: { v: 'version' } },
};

Repro before / after

Probe: require('inquirerer'), then in setImmediate dump process._getActiveHandles() grouped by constructor name. Stdin piped (echo '' | node probe.js) so fd 0 is PIPEWRAP, matching a Jest worker.

Socket (stdin) WriteStream (stdout)
before 1 1
after 0 0

Regression test

Added <ref_file file="/home/ubuntu/repos/dev-utils/packages/inquirerer/tests/no-eager-stdio.test.ts" /> — three assertions:

  1. Object.getOwnPropertyDescriptor(defaultCLIOptions, 'input'|'output') is a getter, not a data property (structural guard — cheapest).
  2. Reading the getters still returns process.stdin / process.stdout (no behaviour regression).
  3. Spawns a fresh node with piped stdin, requires the built dist/index.js, and asserts _getActiveHandles() contains no Socket/WriteStream entries (end-to-end guard that survives future refactors).

All 9 suites / 153 tests pass locally on constructive-io/dev-utils main.

Review & Testing Checklist for Human

  • Sanity-check the getter conversion in <ref_snippet file="/home/ubuntu/repos/dev-utils/packages/inquirerer/src/commander.ts" lines="19-39" /> — I preserved the CLIOptions interface shape (plain Readable/Writable) because TS treats getters as equivalent to data properties for structural typing; make sure nothing in your downstream consumers assumes Object.keys(defaultCLIOptions) enumerates input/output as own-data properties. (Getters are still own and enumerable, so Object.keys/{...defaultCLIOptions} both continue to include them — but worth a glance.)
  • Verify the end-to-end regression test in <ref_file file="/home/ubuntu/repos/dev-utils/packages/inquirerer/tests/no-eager-stdio.test.ts" /> isn't flaky on slower CI — it spawns a child node process. Timed at ~150ms locally.
  • Confirm you'd like a patch release (4.7.1) cut from this so peptide-app-demo → pgsql-test picks up the fix without waiting for a minor bump.

Notes

  • No behaviour change for any consumer that reads defaultCLIOptions.input / .output — they still return process.stdin / process.stdout verbatim.
  • The CLI constructor destructures these at call-time (<ref_snippet file="/home/ubuntu/repos/dev-utils/packages/inquirerer/src/commander.ts" lines="51-58" />), so the only observable difference is when stdin is opened: at new CLI() rather than at import 'inquirerer'.
  • Once this merges + publishes, the --detectOpenHandles warning in peptide-app-demo/packages/integration-tests disappears with no change in that repo; the Jest scripts can keep --detectOpenHandles on as a guard against new leaks without false positives from this chain.
  • I also cherry-picked the same fix into pyramation/inquirerer (the 2.x homepage) locally — happy to open a PR there too if you want the public-mirror repo to stay in sync, but I didn't have push access so it's currently just a local branch.

Link to Devin session: https://app.devin.ai/sessions/81001448b7c54b8099437f706d44a30d
Requested by: @pyramation

Evaluating `input: process.stdin` / `output: process.stdout` as plain
properties on the `defaultCLIOptions` object literal meant those streams
were constructed at module-load time, not when a `CLI` was actually
instantiated.

`process.stdin` and `process.stdout` are lazy getters on the process
object that materialise Readable/Writable streams on first access. When
fd 0 is a pipe (Jest workers, spawned children, backgrounded processes),
constructing stdin registers a libuv PIPEWRAP handle that the runtime
must close before exit. As a result, any library that transitively
required `inquirerer` (e.g. pgsql-test -> pgsql-client -> @pgpmjs/core
-> genomic -> inquirerer) would open stdin as a pure import side-effect
and trigger false positives under `jest --detectOpenHandles`.

Convert the two fields to accessor properties so the stream references
are only evaluated when someone reads them (i.e. when `new CLI(...)`
actually runs). No behavioural change for direct CLI consumers; the
fields still return `process.stdin` / `process.stdout` on read.

Add a regression test that (1) asserts the descriptors are getters and
(2) spawns a fresh Node process with piped stdin, requires the built
package, and verifies no Socket/WriteStream handles remain active.
@devin-ai-integration
Copy link
Copy Markdown

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant